Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add RemoteTransactionProver struct #921

Merged
merged 11 commits into from
Oct 16, 2024
Merged

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Oct 14, 2024

Adds RemoteTransactionProver struct to the miden-tx.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left just a few small comments inline.

One other thing I was wondering: since this is not not just a pure binary - should it still be in the bin directory? Probably not something we need to address as a part of this PR, but maybe we should move it to a crates directory (together with other crates at the root level). Let's create an issue for this.

bin/tx-prover/Cargo.toml Outdated Show resolved Hide resolved
miden-tx/src/errors/mod.rs Outdated Show resolved Hide resolved
miden-tx/src/errors/mod.rs Outdated Show resolved Hide resolved
Comment on lines +27 to +33
[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies]
tonic-web-wasm-client = { version = "0.6", default-features = false }
tonic = { version = "0.12", default-features = false, features = ["prost", "codegen"] }
getrandom = { version = "0.2", features = ["js"], optional = true }

[target.'cfg(not(all(target_arch = "wasm32", target_os = "unknown")))'.dependencies]
tonic = { version = "0.12", default-features = false, features = ["prost", "codegen", "transport"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: does target_os = "unknown" basically mean any OS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to mean any OS, we would not specify target_os (ie, [target.'cfg(all(target_arch = "wasm32"))'.dependencies]). I could be wrong, but I think target_os = "unknown" basically means any environment "without" OS. That is, for instance, for WASI environments this dependencies separator would not match. I added the target_os because of the tonic-web-wasm-client doc which mentions it supports this target explicity, but I think it would not matter much in practice if we removed it.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

miden-tx/src/errors/mod.rs Outdated Show resolved Hide resolved
@igamigo igamigo merged commit 0b13efe into next Oct 16, 2024
8 checks passed
@igamigo igamigo deleted the igamigo-remote-prover branch October 16, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants